gh-131798: JIT: Propagate the result in _BINARY_OP_SUBSCR_TUPLE_INT
#133003
Conversation
Python/optimizer_bytecodes.c
Outdated
| assert(index >= 0); | ||
| assert(index < sym_tuple_length(left)); |
There was a problem hiding this comment.
I'm not sure these asserts are actually needed. The instruction will deopt when this is not true so perhaps we can remove them?
There was a problem hiding this comment.
Interesting, we are actually hitting the assertion:
Python/optimizer_cases.c.h:630: optimize_uops: Assertion `index < sym_tuple_length(left)' failed.I guess I need to move the DEOPT_IF checks into a separate guard for it to work?
There was a problem hiding this comment.
Check out the code for sym_tuple_length. It can return -1 if the length is not known. So for example if you propagate a PyTuple_Type, but not the length, it will fail.
You need to check that the length is not -1.
There was a problem hiding this comment.
Got it, thanks! I somehow didn't realize you can know something is a tuple but not know its length..
Python/optimizer_bytecodes.c
Outdated
| long index = PyLong_AsLong(sym_get_const(ctx, right)); | ||
| assert(index >= 0); | ||
| assert(index < sym_tuple_length(left)); | ||
| res = sym_tuple_getitem(ctx, left, index); |
There was a problem hiding this comment.
This is a bit unfortunate. I would wish we could automatically evaluate this, but it seems tuples are a special category so we can't.
There was a problem hiding this comment.
I haven't fully digested your PR yet, but maybe there is some way to extend it to support tuples as well?
|
Thanks! |
| if (sym_is_const(ctx, sub_st)) { | ||
| assert(PyLong_CheckExact(sym_get_const(ctx, sub_st))); | ||
| long index = PyLong_AsLong(sym_get_const(ctx, sub_st)); | ||
| assert(index >= 0); | ||
| int tuple_length = sym_tuple_length(tuple_st); | ||
| if (tuple_length == -1) { | ||
| // Unknown length | ||
| res = sym_new_not_null(ctx); | ||
| } | ||
| else { | ||
| assert(index < tuple_length); | ||
| res = sym_tuple_getitem(ctx, tuple_st, index); | ||
| } | ||
| } | ||
| else { | ||
| res = sym_new_not_null(ctx); | ||
| } |
There was a problem hiding this comment.
@tomasr8, this can be improved to handle abstract tuples that may not be constant (and it cleans things up, since it includes a length check):
| if (sym_is_const(ctx, sub_st)) { | |
| assert(PyLong_CheckExact(sym_get_const(ctx, sub_st))); | |
| long index = PyLong_AsLong(sym_get_const(ctx, sub_st)); | |
| assert(index >= 0); | |
| int tuple_length = sym_tuple_length(tuple_st); | |
| if (tuple_length == -1) { | |
| // Unknown length | |
| res = sym_new_not_null(ctx); | |
| } | |
| else { | |
| assert(index < tuple_length); | |
| res = sym_tuple_getitem(ctx, tuple_st, index); | |
| } | |
| } | |
| else { | |
| res = sym_new_not_null(ctx); | |
| } | |
| long index = PyLong_AsLong(sym_get_const(ctx, | |
| sub_st)); | |
| // ...PyLong_AsLong can fail, need to check for error here... | |
| res = sym_tuple_getitem(ctx, tuple_st, index) |
There was a problem hiding this comment.
this can be improved to handle abstract tuples that may not be constant
@brandtbucher, I think this already handles abstract tuples? Assuming that abstract means those that have JIT_SYM_TUPLE_TAG rather than JIT_SYM_KNOWN_VALUE_TAG? For tuples that just have JIT_SYM_KNOWN_CLASS_TAG we don't know the length so there's nothing we can do I believe.
But yeah, we can simplify the code since _Py_uop_sym_tuple_getitem can handle unknown length. The only difference is that it sets the type to unknown rather than non null as we do here. Is that a problem? Shouldn't _Py_uop_sym_tuple_getitem always return at least JIT_SYM_NON_NULL_TAG?
There was a problem hiding this comment.
Your current code is gated on sym_is_const, which abstract tuples fail. The rest of your understanding is correct, though.
Is that a problem? Shouldn't
_Py_uop_sym_tuple_getitemalways return at leastJIT_SYM_NON_NULL_TAG?
Sounds like you found your next PR. :)
There was a problem hiding this comment.
Ah, I completely misunderstood, sorry. I thought you were checking whether the tuple was const, not the index. Disregard!
There was a problem hiding this comment.
no worries :) I think we can still simplify the code a bit though!
| if (tuple_length == -1) { | ||
| // Unknown length | ||
| res = sym_new_not_null(ctx); | ||
| } |
There was a problem hiding this comment.
@brandtbucher
Following the previous discussion, I'm wondering if it is correct to return sym_new_not_null here rather than sym_new_unknown?
We have an abstract tuple and we don't know its length so it is impossible to be sure that the index is not out of bounds for the tuple. Given that, I think it is incorrect to return sym_new_not_null here as regular Python code could raise an IndexError. Should we change this to sym_new_unknown?
3 participants
More context: #132851 (comment)
This propagates the information about tuple elements after
_BINARY_OP_SUBSCR_TUPLE_INTwhen the RHS is a constant.For example in
we can now deduce that
xis1.